-
-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor toJtdForm() methods to use centralized converter with switch statement
#26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… statement Co-authored-by: streamich <9773803+streamich@users.noreply.github.com>
toJtdForm() methodstoJtdForm() methods to use centralized converter with switch statement
streamich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run yarn prettier in the end to format codebase.
src/jtd/converter.ts
Outdated
| /** | ||
| * JTD form converter for BooleanType. | ||
| */ | ||
| export function convertBooleanType(type: BooleanType): jtd.JtdTypeForm { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline all individual type converters, such as convertBooleanType, inside the main toJtdForm function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. All individual type converters have been inlined into the main toJtdForm function switch statement. (2088833)
src/jtd/converter.ts
Outdated
| case 'bin': | ||
| return convertBinaryType(type as BinaryType<any>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap each case in a block.
| case 'bin': | |
| return convertBinaryType(type as BinaryType<any>); | |
| case 'bin': { | |
| return convertBinaryType(type as BinaryType<any>); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Each case is now wrapped in a block with curly braces. (2088833)
src/jtd/converter.ts
Outdated
| export function toJtdForm(type: AbstractType<any>): jtd.JtdForm { | ||
| const typeName = type.getTypeName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this method work Schema instead, which is returned from AbstractType.getSchema().
| export function toJtdForm(type: AbstractType<any>): jtd.JtdForm { | |
| const typeName = type.getTypeName(); | |
| export function toJtdForm(schema: Schema): jtd.JtdForm { | |
| const typeName = schema.kind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The method now works with Schema instead of AbstractType and uses schema.kind to determine the type. (2088833)
…s in blocks Co-authored-by: streamich <9773803+streamich@users.noreply.github.com>
Done. Formatted the codebase using |
streamich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run yarn prettier in the end.
src/jtd/converter.ts
Outdated
| switch (numSchema.format) { | ||
| case 'u8': | ||
| return {type: 'uint8'}; | ||
| case 'u16': | ||
| return {type: 'uint16'}; | ||
| case 'u32': | ||
| return {type: 'uint32'}; | ||
| case 'i8': | ||
| return {type: 'int8'}; | ||
| case 'i16': | ||
| return {type: 'int16'}; | ||
| case 'i32': | ||
| return {type: 'int32'}; | ||
| case 'f32': | ||
| return {type: 'float32'}; | ||
| default: | ||
| return {type: 'float64'}; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a map:
| switch (numSchema.format) { | |
| case 'u8': | |
| return {type: 'uint8'}; | |
| case 'u16': | |
| return {type: 'uint16'}; | |
| case 'u32': | |
| return {type: 'uint32'}; | |
| case 'i8': | |
| return {type: 'int8'}; | |
| case 'i16': | |
| return {type: 'int16'}; | |
| case 'i32': | |
| return {type: 'int32'}; | |
| case 'f32': | |
| return {type: 'float32'}; | |
| default: | |
| return {type: 'float64'}; | |
| } | |
| return {type: NUMS_TYPE_MAPPING.get(numSchema.format) ?? 'float64'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Replaced the switch statement with a Map lookup for cleaner code. (0da55d2)
src/jtd/converter.ts
Outdated
| case 'or': { | ||
| // OrType is complex - for simplicity, convert to empty form | ||
| // In a full implementation, this might need to be a discriminator form | ||
| const form: jtd.JtdEmptyForm = {nullable: false}; | ||
| return form; | ||
| } | ||
| case 'bin': { | ||
| // Binary types don't have a direct JTD equivalent, fall back to default | ||
| const form: jtd.JtdEmptyForm = {nullable: false}; | ||
| return form; | ||
| } | ||
| case 'fn': { | ||
| // Function types don't have a direct JTD equivalent, fall back to default | ||
| const form: jtd.JtdEmptyForm = {nullable: false}; | ||
| return form; | ||
| } | ||
| case 'fn$': { | ||
| // Streaming function types don't have a direct JTD equivalent, fall back to default | ||
| const form: jtd.JtdEmptyForm = {nullable: false}; | ||
| return form; | ||
| } | ||
| default: { | ||
| const form: jtd.JtdEmptyForm = {nullable: false}; | ||
| return form; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove comments.
- Combine switch statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed comments and combined the switch statements for unsupported types (or, bin, fn, fn$) into a single case with fallthrough. (0da55d2)
…switch cases, and format with prettier Co-authored-by: streamich <9773803+streamich@users.noreply.github.com>
Done. Formatted the entire codebase using |
streamich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR refactors the JTD (JSON Type Definition) conversion logic as requested in the issue. Previously, each type class had its own
toJtdForm()method implementation scattered across the codebase. This change centralizes all JTD conversion logic into a dedicated module with a clean router function.Changes Made
Created centralized JTD converter module:
src/jtd/converter.tswith individual converter functions for each type (AnyType, BooleanType, ConstType, NumberType, StringType, ArrayType, ObjectType, TupleType, MapType, RefType, OrType, BinaryType, FunctionType)toJtdForm()that uses a switch statement to route each node type to its appropriate convertersrc/jtd/index.tsto export the new converter functionalityRemoved scattered
toJtdForm()methods:AbstractType.toJtdForm()to delegate to the centralized converter using dynamic import to avoid circular dependenciesPreserved exact functionality:
src/jtd/__tests__/converter.spec.ts) to verify all conversions work correctlyThe refactor improves code organization by following the single responsibility principle and makes the JTD conversion logic more maintainable by centralizing it in one location with a clear switch-based routing mechanism.
Fixes #25.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.